[llvm] allow building with builtin_llvm=OFF in Ubuntu26#22513
[llvm] allow building with builtin_llvm=OFF in Ubuntu26#22513ferdymercury wants to merge 4 commits into
Conversation
find_package(LLVM REQUIRED CONFIG) does not work otherwise, because the Find-script is rather strict, it only returns true if major AND minor versions are the same, meaning that API compatibility is ensured. Without this change, in Ubuntu26 I get: couldn't find LLVM, candidate version 20.1.8 does not match required version. Setting a range 20...21 does not help either, it needs exact match of minor and major.
Test Results 22 files 22 suites 3d 15h 25m 27s ⏱️ For more details on these failures, see this check. Results for commit c766d9e. ♻️ This comment has been updated with latest results. |
|
this is a good initiative. Once ubu26 failed, I stopped the build. Maybe, for the tests done in this PR, would it make sense to reduce the number of builds to the ones necessary to check the change? Then, when happy, the full battery can be used before the actual merge. |
|
Thanks! I have tested the Ubuntu26 build like this locally by downloading the docker image
|
|
So far, we did not implement any mechanism to restrict the number of platforms to keep the system simple and maximise coverage. Once upon at time, this was possible, and it did not help development much: in order to save time not all platforms were tested, and that led to breakage. Said that, I think that if this works well, it well could, it ought to become a special build. It is a big change and we might not want (yet) to affect the binaries created for ubuntu 26 nor add the |
|
we'll also have to remember that that hypothetical build, will cease to function once LLVM22 is merged, for obvious reasons, and perhaps we want it also in the CI of the 6.40 branch, again for obvious reasons :-) |
|
Indeed A special build sounds great! It's just for testing not for changing binaries |
| endif() | ||
| else() | ||
| find_package(LLVM REQUIRED CONFIG) | ||
| find_package(LLVM ${ROOT_LLVM_VERSION_REQUIRED_MAJOR}.${ROOT_LLVM_VERSION_REQUIRED_MINOR} REQUIRED CONFIG) |
There was a problem hiding this comment.
I'm not sure about this change: You're saying that the previous line find_package(LLVM REQUIRED CONFIG) doesn't work at all? It is the officially documented way to find LLVM: https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project Then we are manually checking the version further below.
There was a problem hiding this comment.
Yes, that's what I am saying/experiencing on Ubuntu 26 root docker image, it does not work at all. If you check the LLVMConfigVersion.cmake file https://github.com/ferdymercury/root/blob/master/interpreter/llvm-project/llvm/cmake/modules/LLVMConfigVersion.cmake.in, it's checking for strict match of major and minor versions. So found version 20.1.8 does not match requested version unspecified.unspecified
It's weird, but they seem to enforce that strict match to be API compatible.
In any case, the solution of adding/specifying major minor when calling find is not too bad, so that we ensure it matches the current builtin llvm.
There was a problem hiding this comment.
Seems it's not a bug but a known "feature": https://stackoverflow.com/questions/66305592/how-to-match-major-version-only-in-cmake
There was a problem hiding this comment.
Seems to work for me when specifying the right variables: #22544
4 participants
builtin_llvm=OFF option is not tested anywhere in the CI.
find_package(LLVM REQUIRED CONFIG) does not work otherwise, because the Find-script is rather strict, it only returns true if major AND minor versions are the same, meaning that API compatibility is ensured.
Without this change, in Ubuntu26 I get:
couldn't find LLVM, candidate version 20.1.8 does not match required version. Setting a range 20...21 does not help either, it needs exact match of minor and major.
depends on root-project/root-ci-images#131